[pkg/ottl]Add trace_state.adjusted_count path to span context #36789
Conversation
github-actions
bot
added
connector/count
connector/datadog
connector/routing
connector/sum
exporter/datadog
github-actions
bot
requested review from
ankitpatel96,
bogdandrutu,
boostchicken,
carsonip,
crobert-1,
dineshg13,
djaglowski,
evan-bradley,
fchikwekwe,
greatestusername,
jade-guiton-dd and
JaredTan95
|
Sorry about the review notifications folks, I was not expecting a new draft PR to add reviewers 😓 |
github-actions
bot
added
processor/cumulativetodelta
|
Bringing the PR out of draft state after getting confirmation on the approach in this comment. |
There was a problem hiding this comment.
Overall this looks okay to me, but I'd like someone more familiar with trace sampling to take a look. @kentquirk would you be willing to review this one?
|
👋 folks, the conflicts in the PR are trivial and will continue to happen with new version ups. To avoid repetitive task, I plan to fix the conflicts once the PR is approved but let me know if you want to have that fixed before reviewing - I can do that too. |
|
@kentquirk If you have a minute, could you please review this? |
| otTraceState := w3cTraceState.OTelValue() | ||
| if otTraceState == nil { | ||
| // If otel trace state is missing, default to 1 | ||
| return float64(1), nil |
There was a problem hiding this comment.
Is this defined by the sampling spec as a default value? If not we should probably error. It feels like accessing the trace state with an unset "key" is an issue.
There was a problem hiding this comment.
Is this defined by the sampling spec as a default value?
I don't think it does but, IIUC, sampling spec will only apply if the OTel trace state is available and I am not sure if OTel state would always be available - I also don't see anything in the trace state handling.
Assuming that OTel trace state is not always present, if we were to error out when the OTel trace state is nil then the function would be far less useful as I would not want the adjusted count to be 0 in this case. Let me know if this makes sense.
There was a problem hiding this comment.
if we were to error out when the OTel trace state is nil then the function would be far less useful as I would not want the adjusted count to be 0 in this case
If we return an error the adjusted_count won't be 0, the statement will fail and be subjected to the configured error mode. Erroring would mean you don't get a value back from adjusted_count that is inaccurate - I am worried if we return a default value of 1 that we could be misleading users. Should we default to unknown?
There was a problem hiding this comment.
Because of tracestate propagation rules that limit the length of the tracestate, It is not an error to fail to find the appropriate values. However, we should return 0 in this case (meaning we have no information regarding adjusted count) rather than 1 (meaning that we have only one representative of this value).
Some users of this API may choose to assume that 0 effectively means 1, but others will prefer to be able to avoid accumulating inaccurate results. I know that means that some people will be forced to add an extra conditional, but I know that both types of users are real.
There was a problem hiding this comment.
Sound like that would make it difficult to differentiate between actual zero values and missing values. What do you think about adding the ability to specify a default value for when the value is not known/missing? I guess it would be difficult in with the current design as you can't pass a parameter to a path expression. But it would work well with a function, as prototyped in #36573.
Description
Introduces
trace_state.adjusted_countpath which calculates adjusted count for an OTTL span.Link to tracking issue
Fixes #36572
Alternative to #36573
Testing
Added unit tests
Documentation